Note that some (actually most) xtensa SOCs don't come through this code, having their own bootstrap path (often coming out of e.g. bootloader ROMs that have already done the hardware setup). If this has complicated things to do we should consider adding a test somewhere to catch platforms that mess it up.

Alternatively, if this is something that isn't required for CPU0 (looks like it isn't) consider moving to arch_kernel_init(), which runs on a cleanly-initialized CPU capable of running C code. (Though I guess this isn't a windowed call so would still need to be done in inline assembly)

Finally, my memory is that PRID is not guaranteed to be equal to zero for the boot CPU. On esp32, my memory (which is fuzzy, Espressif folks will want to comment) is that both cores had non-zero/not-even-small-integer PRID values. This may not work the way you want, which would require putting this code on the SMP bringup path.

Also: um... what exactly does "init bss" even mean? Is .bss not expected to be zero-filled?!

Sorry, correcting typo/ambiguity: "isn't required for CPU1+". That is, this looks like code you want to run on the boot CPU only, after startup but before any subsystem code that might touch the libc runs. That's exactly what arch_kernel_init() is for.

I successfully verified moving the _init_reent_bss() call to arch_kernel_init(), but in an SMP SoC is arch_kernel_init() meant to run only on the boot CPU ? if not, is there an existing CONFIG_ macro which has the ID of the boot CPU ?

_init_reent_bss() is a bit of misnomer; _init_reent_bss() is to be called to initialize the single global copy of struct _reent state for single-threaded executables.

With the latest update of this PR, calling _init_reent_bss() is no longer needed.
__DYNAMIC_REENT__ support in xclib is used whereby the _reent struct is retrieved using __getreent().

@andyross Is arch_kernel_init() guaranteed to run only on the boot CPU ?
if not, is there an existing CONFIG_ macro which has the ID of the boot CPU ?

This PR no longer modifies the file arch/xtensa/include/kernel_arch_func.h which holds arch_kernel_init().

But I would appreciate a response on whether arch_kernel_init() is guaranteed to run only on the boot CPU.

Seems awkward to do this in assembly in the darkest center of the context switch. It's just a C function call, why not add e.g. _xclib_update_reent_ptr(_current); to arch_switch(), which is the inline wrapper around this. It's currently degenerate but at some point in the past had C code in it.

Also unless I'm missing it, this needs to be added to the context switch path on interrupt return too, no?

Actually reading that, I think the argument wants to be &_current->reent and not the bare thread pointer, right? The libc obviously doesn't know the layout of a zephyr thread, it wants that struct _reent I assume?

this looks just like newlib; can it use the same bits as that?

@andyross correct I need the reent structure address of the current thread to update the global struct _reent *_reent_ptr used by xclib for threadsafety.

@keith-packard please could you point me to the sources for newlib ?

The newlib integration bits for Zephyr are in lib/libc/newlib; you can either copy all of that and adjust to suit or try to share the code between newlib and the Xtensa toolchain.

@keith-packard The integration bits do not seem to implement anything for managing the current struct _reent.
I would have expected to see __DYNAMIC_REENT or _REENT_THREAD_LOCAL set.

With the latest update of this PR, __DYNAMIC_REENT__ support in xclib is used whereby the _reent struct is retrieved using __getreent().
It is no longer needed to do a _reent pointer update during context switch.

@keith-packard @andyross To further test SMP thread safety, I am using tinyraytracer which does ray-tracing and render on the terminal:
https://github.com/BrunoLevy/TinyPrograms/blob/main/tinyraytracer.c

Each pixel is computed independently, so I saw an opportunity to use it as a multi-threading test.

I ported it to Zephyr, where as many thread as there are cores available get created:
williamtcdns@35d41e30

Both Xtensa C library and PICOLIBC successfully complete the rendering for a single core:

However, with more than one cores (ie: 4 cores in my tests), PICOLIBC fails, while I somewhat (noise in the rendering being debugged) get a complete rendering with Xtensa C library because it manages the current struct _reent:

If the above Zephyr port of tinyraytracer sounds like an interesting SMP thread safety test, I would be more than happy to submit it as a sample test.

Not relevant to this PR, but note that once this lands SOF will want to use it instead of its current integration for a handful of third party C++ modules, which add the libraries manually. (@lyakh @kv2019i et. al.)

Don't use a SYS_INIT() hook for arch code, it wastes two words for the record and however many instructions it takes for that wrapper that adds a needless return value. Arch code has copious spots to put initialization code in before calling z_cstart(), c.f. the arch_kernel_init() suggestion above.

Also, note that the "APPLICATION" init level is very late, and there may be driver/subsystem code from many sources (including out-of-the-zephyr-tree) that relies on libc behavior and whatever _Initlocks() needed to do. If for some reason you do need this to be a SYS_INIT() hook, I'm guessing you want it to be PRE_KERNEL.

Thanks, I also successfully verified moving the _Initlocks() call to arch_kernel_init().

With the latest update of this PR, calling _Initlocks() is no longer needed.
In fact, this PR no longer modifies the file arch/xtensa/include/kernel_arch_func.h which holds arch_kernel_init().

Pet peeve: you probably want a spinlock or a k_sem here and not a mutex. k_mutex is very heavyweight and the only feature it brings is priority inheritance. Semaphores are a better general purpose blocking IPC lock for almost all purposes. But given the use case, I'm thinking that the libc is probably using this to protect tight critical sections, where spinlocks are much lighter still and objectively better choices.

I initially used k_sem, but a recursive locking mechanism is needed to avoid hangs.